Skip to content

tls: match IPv6 hosts against IP-Address SANs#64145

Open
JumpLink wants to merge 1 commit into
nodejs:mainfrom
JumpLink:tls-ipv6-ip-san
Open

tls: match IPv6 hosts against IP-Address SANs#64145
JumpLink wants to merge 1 commit into
nodejs:mainfrom
JumpLink:tls-ipv6-ip-san

Conversation

@JumpLink

@JumpLink JumpLink commented Jun 26, 2026

Copy link
Copy Markdown

tls.checkServerIdentity() stopped matching an IPv6 host against a matching IP Address SAN, returning ERR_TLS_CERT_ALTNAME_INVALID (Cert does not contain a DNS name) where it used to return undefined.

The hostname is now run through domainToASCII() before the net.isIP() gate, and domainToASCII('::1') === '' (an IPv6 literal is not a domain), so net.isIP('') is 0, the IP-SAN branch is skipped, and (with no DNS SAN / CN) it falls through to the no-identifier reason. IPv4 is unaffected because dotted-decimal survives domainToASCII().

const tls = require('node:tls');
tls.checkServerIdentity('::1', { subject: {}, subjectaltname: 'IP Address:::1' });
// v24.16.0: undefined (matched)   v24.17.0+/v26.x: ERR_TLS_CERT_ALTNAME_INVALID

This matches IP hosts against the original hostname instead of the IDNA-normalized one. net.isIP() rejects non-ASCII input, so there is no IDNA confusion to guard against for an IP literal; the normalized form is still used for the DNS-name path. Adds IPv6 IP-SAN coverage (match, canonical-form match, non-match) to test-tls-check-server-identity.

Fixes: #64144

@nodejs-github-bot

nodejs-github-bot commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Jun 26, 2026
@JumpLink

JumpLink commented Jun 26, 2026

Copy link
Copy Markdown
Author

@pimterry sorry for the extra ping! The lint-commit-message check was failing because the first commit was missing the DCO Signed-off-by trailer, so I force-pushed to add it. That reset the GitHub Actions approval, and the workflow runs are now sitting at action_required. Would you mind approving the workflow runs again so the full CI can run? Thanks a lot for the earlier approval and for the review!

@pimterry

pimterry commented Jun 29, 2026

Copy link
Copy Markdown
Member

Can you rebase this again please @JumpLink? Sorry, there was an issue on main due to two conflicting PRs merging. It's resolved on main, but as you can see it breaks the tests here.

@JumpLink

JumpLink commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

@pimterry Done, thanks for the heads-up about the conflicting PRs 👍

@pimterry

pimterry commented Jul 7, 2026

Copy link
Copy Markdown
Member

Sorry this stalled @JumpLink. The failure is spurious I think, it seems there was another test issue (sigh) with coverage-windows test and GH runner updates. Can you rebase and force push again? The failure is fixed in f1e01e7 I think.

@JumpLink
tls: match IPv6 hosts against IP-Address SANs
1a0b0f4 
checkServerIdentity() stopped matching an IPv6 host against a matching
IP-Address SAN. The hostname is now run through domainToASCII() before
the net.isIP() gate, and domainToASCII('::1') === '' (an IPv6 literal is
not a domain), so net.isIP('') is 0, the IP-SAN branch is skipped, and
verification fails with "Cert does not contain a DNS name". IPv4 is
unaffected because dotted-decimal survives domainToASCII().

Match IP hosts against the original hostname instead of the IDNA-
normalized one. net.isIP() rejects non-ASCII input, so there is no IDNA
confusion to guard against for an IP literal; the normalized form is
still used for the DNS-name path.

Fixes: nodejs#64144
Signed-off-by: Pascal Garber <pascal@artandcode.studio>
@JumpLink

JumpLink commented Jul 7, 2026

Copy link
Copy Markdown
Author

@pimterry Rebased onto latest main. This pulls in #64222 ("build: suppress clang errors building libffi on Windows"), which is what was turning the coverage-windows build red on the previous run — my earlier base predated that fix.

Since the force-push reset the workflow approval, could you kick the CI off again when you get a chance? Thanks!

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2026
@nodejs-github-bot

nodejs-github-bot commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74644/

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.25%. Comparing base (3ee5f31) to head (1a0b0f4).
⚠️ Report is 45 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #64145      +/-   ##
==========================================
- Coverage   92.04%   90.25%   -1.80%     
==========================================
  Files         381      741     +360     
  Lines      169252   240985   +71733     
  Branches    25941    45403   +19462     
==========================================
+ Hits       155792   217501   +61709     
- Misses      13173    15057    +1884     
- Partials      287     8427    +8140
Files with missing lines Coverage Δ
lib/tls.js 93.13% <100.00%> (+1.34%) ⬆️

... and 500 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pimterry pimterry pimterry approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tls: checkServerIdentity() no longer matches IPv6 IP-Address SANs (regressed in v24.17.0)

3 participants

@JumpLink @nodejs-github-bot @pimterry